Skip to content

Improve Action trait #15

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Improve Action trait #15

wants to merge 3 commits into from

Conversation

mxgrey
Copy link
Contributor

@mxgrey mxgrey commented Aug 10, 2025

This PR introduces some recommended changes for the Action trait based on what I encountered while finishing the implementation of action clients and servers in rclrs. When the Action trait was first introduced to the library, it was largely inspired by how the template implementation of rclcpp_action works. This was reasonable for the most part, but some aspects don't line up perfectly with what makes the most sense in practical Rust.

Here are the changes introduced by this PR and their motivation:

  • ActionImpl is removed. The C++ templates have a separation of public API and private API for details related to the implementation of actions. This makes sense from a software engineering perspective, and it would be nice to have a clean separation in rclrs as well, but in practice I found that it didn't accomplish anything. Because of how generics work in Rust, I wound up needing to use the trait bound A: ActionImpl everywhere in rclrs, to the point that the Action trait was never used. At that point I figured we're not achieving a separation anyway if we need to use ActionImpl everywhere, so we may as well roll it all into simply Action.
  • split_ instead of get_ methods. Various action service messages consist of two fields mashed together into one message, and we need to use associated functions to separate those fields. The current main uses methods that give back a borrow of each field, but this forces a .clone() which can be avoided by splitting the message instead of borrowing it. In practice I found no value in borrowing the fields while implementing actions services and clients.
  • Expose Rmw... type aliases. These type aliases were declared pub but were inaccessible outside of the crate because of how the trait module was set up. These aliases are useful in rclrs, so I've updated the pub use traits to include everything in the module. I saw no reason to cherry-pick the module contents.

This PR is a dependency of ros2-rust/rosidl_rust#7 and ros2-rust/ros2_rust#503

mxgrey added 2 commits July 28, 2025 15:11
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant